Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Propagate block hashes instead of full blocks (network breaking) #1986

Merged
merged 1 commit into from
Jun 18, 2020

Conversation

sdbondi
Copy link
Member

@sdbondi sdbondi commented Jun 12, 2020

Description

  • Nodes propagate a block hash (NewBlock message = 33 bytes) instead
    of the full block. This will significantly reduce network bandwidth
    usage for block propagation.
  • On receipt of a block hash a node checks if it has the block.
    If so, it simply ignores the message. Otherwise, it requests the
    full block from the peer that sent the NewBlock message. If the
    block is valid and has been added to the node's blockchain db,
    the node propagates the block hash message to other peers.
  • Change block propagate test to tests a few block hashes being propagated rather
    than a single block.
  • Changed invalid block test to check that an invalid block is not
    propagated.
  • New test to check that an invalid block hash is not propagated
  • Cleaned up some necessary generics on structs

Motivation and Context

Significantly reduce the bandwidth required for block propagation.
The benefits are twofold:

  1. Outgoing: The size of the message that is propagated in one round is many orders of magnitude smaller
  2. Incoming: Previously, a node may receive a full block many times. With this change a node will receive the block exactly once.

Possible issues:

  1. The InboundNodeCommsHandler::handle_new_block_message function is able to correctly handle duplicates by sequentially querying all nodes that sent the block hash message. However, due to message dedup, the message is only received once. If the full block is not returned from the source node (for whatever reason) then the node will miss the block and have to transition to block sync state to catch up.

How Has This Been Tested?

  • Invalid block propagation test changed to propagate a hash that exists and have Alice return an invalid block (or rather a block that is invalid to Bob and Carol).
  • Test the behaviour when a block hash is propagated for which Alice cannot return a block.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Feature refactor (No new feature or functional changes, but performance or technical debt improvements)
  • New Tests
  • Documentation

Checklist:

  • I'm merging against the development branch.
  • I ran cargo-fmt --all before pushing.
  • I have squashed my commits into a single commit.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@sdbondi sdbondi force-pushed the sb-core-block-hash-proto branch 2 times, most recently from 7d223ce to de31f7e Compare June 12, 2020 18:01
CjS77
CjS77 previously approved these changes Jun 14, 2020
Copy link
Collaborator

@CjS77 CjS77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Should not merge until we're ready for 0.4.0 release

- Nodes propagate a block hash (`NewBlock` message = 33 bytes) instead
  of the full block.  This will significantly reduce network bandwidth
  usage for block propagation.
- On receipt of a block hash a node checks if it has the block.
  If so, it simply ignores the message. Otherwise, it requests the
  full block from the peer that sent the `NewBlock` message. If the
  block is valid and has been added to the node's blockchain db,
  the node propagates the block hash message to other peers.
- Change block propagate test to tests a few block hashes being propagated rather
  than a single block.
- Changed invalid block test to check that an invalid block is not
  proagated.
- New test to check that an invalid block hash is not propagated
- Cleaned up some unecessary generics on structs
@sdbondi sdbondi force-pushed the sb-core-block-hash-proto branch 2 times, most recently from 0f5e66d to 4159dc3 Compare June 18, 2020 10:17
@CjS77 CjS77 merged commit 9c9c602 into development Jun 18, 2020
@CjS77 CjS77 deleted the sb-core-block-hash-proto branch June 24, 2020 08:41
sdbondi added a commit to sdbondi/tari that referenced this pull request Sep 16, 2020
This PR changes the `Flood` broadcast stragey to "flood" a message to
every _connected_ peer node. Since PR tari-project#1986, block hashes
are propagated across the network and the full block only downloaded
once per peer (push became push-pull gossip). This large drop in propagation
overhead means that we can afford to spread the hash a little more
widely and use the modified `Flood` stragegy.

The main change is to change the flood strategy from sending to _all_
valid peers in the peer list, to only sending to currenty connected
peers (inbound and outbound connections). As before, client nodes are
exempt from flood propagation messages.
sdbondi added a commit to sdbondi/tari that referenced this pull request Sep 16, 2020
This PR changes the `Flood` broadcast stragey to "flood" a message to
every _connected_ peer node. Since PR tari-project#1986, block hashes
are propagated across the network and the full block only downloaded
once per peer (push became push-pull gossip). This large drop in propagation
overhead means that we can afford to spread the hash a little more
widely and use the modified `Flood` stragegy.

The main change is to change the flood strategy from sending to _all_
valid peers in the peer list, to only sending to currenty connected
peers (inbound and outbound connections). As before, client nodes are
exempt from flood propagation messages.
sdbondi added a commit to sdbondi/tari that referenced this pull request Sep 16, 2020
This PR changes the Flood broadcast stragey to "flood" a message to
every connected peer node. Since PR tari-project#1986, block hashes
are propagated across the network and the full block only downloaded
once per peer (push became push-pull gossip). This large drop in propagation
overhead means that we can afford to spread the hash a little more
widely and use the modified Flood strategy. Previously, the hash propagation
used the propagate strategy which only sends to 4 random connected peers.

The flood strategy is changed from sending all valid peers in the peer list,
to only sending to currently connected peers (inbound and outbound connections).
As before, client nodes are exempt from flood propagation messages.
stringhandler added a commit that referenced this pull request Sep 22, 2020
This PR changes the (previously unused) Flood broadcast strategy to send a message to
every connected peer node. Since PR #1986, block hashes
are propagated across the network and the full block is only downloaded
once per peer (push became push-pull gossip). This large drop in propagation
overhead means that we can afford to spread the hash a little more
widely and use the modified Flood strategy. Previously, the hash propagation
used the propagate strategy which only sends to 4 random connected peers.
StriderDM pushed a commit to StriderDM/tari that referenced this pull request Oct 2, 2020
This PR changes the Flood broadcast stragey to "flood" a message to
every connected peer node. Since PR tari-project#1986, block hashes
are propagated across the network and the full block only downloaded
once per peer (push became push-pull gossip). This large drop in propagation
overhead means that we can afford to spread the hash a little more
widely and use the modified Flood strategy. Previously, the hash propagation
used the propagate strategy which only sends to 4 random connected peers.

The flood strategy is changed from sending all valid peers in the peer list,
to only sending to currently connected peers (inbound and outbound connections).
As before, client nodes are exempt from flood propagation messages.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants